publish: return api.ErrCanceled when user declines interactive prompts#13674
Conversation
There was a problem hiding this comment.
Pull request overview
This PR changes compose publish to return a detectable error when the user declines interactive safety prompts, instead of returning nil (success), so automation/callers can distinguish an abort from a successful publish.
Changes:
- Introduce a new sentinel error (
ErrPublishAborted) and return it when pre-check prompts are declined. - Add unit tests intended to cover “decline” paths for
preChecksandpublish.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/compose/publish.go | Returns a sentinel error when publish is aborted via interactive decline. |
| pkg/compose/publish_test.go | Adds tests for decline behavior in pre-checks and publish. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ishwar170695
force-pushed
the
fix/publish-aborted-sentinel-error-v2
branch
from
dc3c9cb to
579bbe7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ishwar170695
changed the title
|
Updated PR description to reflect the use of the existing api.ErrCanceled instead of creating a new custom error. |
glours
left a comment
glours
left a comment
There was a problem hiding this comment.
Thanks for the contribution 🙏
I have small requests before we can merge it
pkg/compose/publish_test.go
Outdated
| }, cmp.Ignore())) | ||
| } | ||
|
|
||
| func Test_preChecks_decline_returns_ErrPublishAborted(t *testing.T) { |
There was a problem hiding this comment.
There isn’t any ErrPublishedArboted error in the codebase, so the test name is misleading.
Second, this test is effectively a subset of Test_publish_decline_returns_ErrCanceled.
We can keep it, but I’d suggest repurposing it to cover the sensitive data detection path instead. That would make it exercise an uncovered code path and give it a clearer purpose.
|
Thanks for the feedback! I've repurposed the test to cover the checkForSensitiveData + decline path using a temp .env file with a dummy AWS secret key. The test name is updated to reflect what it actually tests. |
|
@ishwar170695 can you sing-off your commits please |
|
@ishwar170695 CI detected tests which need to be changed to check properly the new 130 exit code |
Signed-off-by: Ishwar <[email protected]>
Renames test to Test_preChecks_sensitive_data_detected_decline. Uses a temporary .env file with an AWS token to reliably trigger the DefangLabs secret detector, and confirms that preChecks correctly aborts early on user decline. Signed-off-by: Ishwar <[email protected]>
Signed-off-by: Ishwar <[email protected]>
Head branch was pushed to by a user without write access
ishwar170695
force-pushed
the
fix/publish-aborted-sentinel-error-v2
branch
from
c8d190e to
df5d68b
Compare
|
Sorry for disabling auto-merge, the force-push after rebase triggered that. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
What I did
Previously, declining the bind mount or sensitive data warning prompt caused publish() to return
nil, which is indistinguishable from success for callers and automation scripts (exit code 0).Change publish() to return the existing
api.ErrCanceledsentinel so callers can detect user-initiated cancellation viaerrors.Is(). Add unit tests covering both thepreChecksdecline path and the fullpublish()path.Signed-off-by: Ishwar chand meena [email protected]
(not mandatory) A picture of a cute animal, if possible in relation to what you did